-
Notifications
You must be signed in to change notification settings - Fork 38.8k
SingletonSupplier is not singleton if null is returned on first supplier call #35380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a722667
to
a596879
Compare
private static final Object NULL = new Object() { | ||
|
||
@Override | ||
public String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that overriding this method is not necessary, what's the purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, SingletonSupplier stored "null" in singletonInstance when the supplied instance was "null". On subsequent get() calls, this was treated as "uninitialized" and triggered another attempt to obtain an instance. This commit ensures that a "null" returned from the instanceSupplier or defaultSupplier is handled correctly, so that subsequent calls to get() return "null" consistently instead of repeatedly invoking the supplier. Signed-off-by: Dmytro Nosan <[email protected]>
|
||
reset(dataSource); | ||
given(dataSource.getConnection()).willReturn(connection); | ||
translator = new SQLErrorCodeSQLExceptionTranslator(dataSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to this issue/change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
public @Nullable T get() { | ||
T instance = this.singletonInstance; | ||
if (instance == null) { | ||
if (!this.initialized) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as initialized
has no external use, is this variable really necessary? cant we directly use this.singletonInstance == null
or add a function->
public boolean isInitialized() {
return Objects.nonNull(this.singletonInstance);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this.singletonInstance == null
can't tell the difference between "not initialized yet" and "initialized but supplier returned null." Since the supplier can return null, a separate flag needs to be used to record that initialization was attempted. Otherwise, the supplier will keep getting re-invoked.
See #35369 for more details.
See #35369